Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix our NBT serde #549

Merged
merged 12 commits into from
Feb 16, 2025
Merged

Fix our NBT serde #549

merged 12 commits into from
Feb 16, 2025

Conversation

kralverde
Copy link
Contributor

Description

Currently our NBT handler is pretty buggy. This PR aims to fix it and add more unit tests.

Testing

Please follow our Coding Guidelines

@kralverde
Copy link
Contributor Author

Conflicts #543; one needs to be merged before the other.

@kralverde kralverde marked this pull request as ready for review February 14, 2025 06:37
@kralverde kralverde marked this pull request as draft February 14, 2025 09:25
@kralverde kralverde mentioned this pull request Feb 14, 2025
Closed
@kralverde
Copy link
Contributor Author

Pulled in tests from #486

@kralverde kralverde marked this pull request as ready for review February 15, 2025 06:28
@Snowiiii
Copy link
Collaborator

Lets remove fastnbt completely, you still have it as dev dependency

@kralverde
Copy link
Contributor Author

Lets remove fastnbt completely, you still have it as dev dependency

It's for tests to compare the output

@kralverde
Copy link
Contributor Author

On second thought I'll just use raw bytes to check

@kralverde
Copy link
Contributor Author

@Snowiiii removed all fast_nbt

@Snowiiii
Copy link
Collaborator

When i tested loading a Vanilla anvil world it worked fine, Until i moved. Then i got

thread '<unnamed>' panicked at /home/alex/Documents/Development/Rust/Pumpkin/pumpkin-nbt/src/deserializer.rs:153:58:
called `Option::unwrap()` on a `None` value

@kralverde
Copy link
Contributor Author

kralverde commented Feb 16, 2025 •
edited
Loading

When i tested loading a Vanilla anvil world it worked fine, Until i moved. Then i got

thread '<unnamed>' panicked at /home/alex/Documents/Development/Rust/Pumpkin/pumpkin-nbt/src/deserializer.rs:153:58:
called `Option::unwrap()` on a `None` value

Was this with a world previously generated by us? Do you have a stack trace? I'm unable to reproduce.

@kralverde
Copy link
Contributor Author

I see; its when we load chunks from java versions, ill take a look

@kralverde
Copy link
Contributor Author

Should be fixed now @Snowiiii

@Snowiiii
Copy link
Collaborator

Great work @kralverde, Everything works fine now. Thank you 👍

@Snowiiii Snowiiii merged commit 7ec99a0 into Pumpkin-MC:master Feb 16, 2025
10 checks passed
urisinger pushed a commit to urisinger/Pumpkin that referenced this pull request Feb 16, 2025
@kralverde @urisinger
Fix our NBT serde (Pumpkin-MC#549)
f6b8c1d 
* make deserializer streamable

* fix logic

* fix deserializing

* work on deserialization

* finish serialize

* replace everything with pumpkin_nbt

* fix more serialization cases

* finish conversion

* add comment

* remove all fast_nbt, test nbt arrays, add implement tuples/arrays

* add size hints for deserialization

* fix deserialization
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kralverde @Snowiiii